feat(l1+evm+precompiles): add L1-backed zone db#698
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
0394607 to
0c94b43
Compare
0c94b43 to
604908a
Compare
|
cyclops audit |
|
cc @0xrusowsky Cyclops audit event published. View workflow run Config: config: |
👁️ Cyclops Security Review
🧭 Finalizing · mode=
Findings
⚙️ Controls
📜 48 events🔍 |
| if let Some(account) = state.get(&TIP403_REGISTRY_ADDRESS) { | ||
| if account.info != account.original_info() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, U256::ZERO); | ||
| } | ||
| for (slot, value) in &account.storage { | ||
| if value.is_changed() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, *slot); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Yeah seems like a reasonable defense in depth.
| } | ||
|
|
||
| let anchor = self.anchor()?; | ||
| let l1 = self.l1_storage(address, slot, anchor)?; |
There was a problem hiding this comment.
This can return stale policy state after the first read. L1StateProvider::get_storage treats the latest cached value at or before anchor as valid, but policy events still update only PolicyCache, they do not write or invalidate the corresponding raw L1StateCache slots.
For example, after a slot is cached at N and changed on L1 at N+1, this read at N+1 returns the N value without making an RPC request. Cache warmth can then make nodes execute the same block with different policy state.
There was a problem hiding this comment.
nice catch, was too aggressive when reverting L1 state cache changes to minimize the diff.
Restored a minimal version which addresses the soundness with e21f5a5 (this PR)
| if storage::merge_transfer_policy_id(U256::ZERO, value.present_value) | ||
| != storage::merge_transfer_policy_id(U256::ZERO, observed.l1) |
There was a problem hiding this comment.
This rejects the mandatory enableToken path whenever the token already has a non-default L1 policy.
ZoneTokenFactory::enable_token calls upstream TIP20Token::initialize, which writes transfer policy ID 1 into this packed slot. If the overlaid L1 field is anything other than 1, this comparison returns an error, causing the entire advanceTempo system transaction to fail and stalling block production.
Initialization needs an exception that discards/restores the mirrored field without treating the upstream default write as an attempted L1 mutation.
There was a problem hiding this comment.
totally missed it, nice catch. Rather than weakening the DB adapter with an initialization exception, enable_token now snapshots the anchored policy field, runs upstream initialization, and restores that field in the journal while preserving the adjacent initialized fields 18a7ded (this PR)
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
This replacement lookup drops the upstream RECEIVE_POLICY_GUARD_ADDRESS precompile. With L1 backed TIP403 execution, upstream TIP20 can enforce a receive policy, move blocked funds into the guard, and record a claim receipt internally.
However, users cannot call the guard's public claim or burnBlockedReceipt methods because this lookup returns None for its address, leaving those funds permanently locked. We should either explicitly reject receive policy resolution or find a different sollution.
There was a problem hiding this comment.
i missed this too. It seems fine to simply enable it in the L2?
| fn decode_and_check<C: SolCall>(args: &[u8], check: impl FnOnce(C) -> CallCheck) -> CallCheck { | ||
| match C::abi_decode_raw_validate(args) { | ||
| Ok(decoded) => check(decoded), | ||
| Err(_) => CallCheck::Continue, | ||
| } | ||
| } |
There was a problem hiding this comment.
Protected selectors must not continue when strict ABI validation fails. Upstream dispatch accepts some encodings that abi_decode_raw_validate rejects.
For example, setting a nonzero byte in an address word's upper padding makes this helper return Continue, while upstream still decodes the address and successfully serves balanceOf.
There was a problem hiding this comment.
mmm i guess it makes sense. Is there a reason why we were using abi_decade_raw_validate here rather than the upstream abi_decade_raw?
if not the easiest fix seems to simply change the fn and continue delegating errors to upstream impl
| block_number, | ||
| address_to_storage_value(pending_sequencer), | ||
| ); | ||
| set_cache_slot(PORTAL_SEQUENCER_SLOT, current_sequencer.into_word()); |
There was a problem hiding this comment.
Can we derive this slot (as well as other slots) from the sol! bindings rather than using a const?
There was a problem hiding this comment.
will do it later in a followup, since it was already the case
| ) -> Result<TempoResult, ZoneEvmError<DB::Error>> { | ||
| let result = match execute(&mut self.inner) { | ||
| Ok(mut result) => { | ||
| if result.result.is_success() |
There was a problem hiding this comment.
Reverts and halts still return state that the block executor commits, including the nonce and Tempo fee transition, so sanitization cannot be success only. B
Before execution, collect_fee_pre_tx authorizes the fee token transfer through TIP403 and mutates that same token's balances/reward state. Those fee effects remain when the user call later reverts.
The returned touched token account contains the L1 overlaid policy bits, but this branch skips sanitize_state, and commit_transaction persists them into canonical Zone storage.
Should we sanitize every Ok(result) regardless of ExecutionResult? The reverted user-frame writes have already been rolled back, while the persistent fee/nonce transition still needs its mirrored fields removed.
| if value.is_changed() { | ||
| return l1_write(TIP403_REGISTRY_ADDRESS, *slot); | ||
| } |
There was a problem hiding this comment.
Checking only is_changed() validates that the call did not modify the L1 value, but it does not remove that value from the returned transition. The adapter supplied the L1 value as both original_value and present_value, and revm commits every storage entry of a touched account.
For example, with canonical Zone slot 0 equal to 5 and the mirrored L1 value equal to 99, a policyIdCounter() transaction succeeds, sanitize_state returns Ok, and the touched registry account in result.state contains 99. Committing that result overwrites the Zone slot even though the transaction was read only.
cc @klkvr to double check if Im overlooking something here.
| pub(crate) struct NoCallRules; | ||
| impl CallRules for NoCallRules { | ||
| fn is_delegate_call_allowed(&self) -> bool { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you remind me why/when we need to delegate call with precompiles? We explictly restrict this upstream in Tempo via the tempo_precompile macro.
There was a problem hiding this comment.
i saw that the L2 crypto precompiles (which are "pure") allowed delegate calls, so i thought u wanted to preserve behavior.
IMO it would be nicer to never allow it, as it would simplify impl
f66d185 to
0abf462
Compare
| /// The execution-local Tempo anchor used by the database adapter. | ||
| #[derive(Clone, Default)] | ||
| pub struct L1AnchorController { | ||
| state: Rc<Cell<L1AnchorPhase>>, | ||
| } |
There was a problem hiding this comment.
is there any way to avoid doing this or simplify? iiuc this is the main trade off vs ZoneStorageProvider which can read tempoBlockNumber from the state directly on every L1 read
in practice during execution zone's EVM only observes L1 state at a single anchor because advanceTempo is called at the beginning of the block so there must be a way to enforce this somehow
There was a problem hiding this comment.
simplified the state machine to a an Rc<Cell<Option<u64>>> and absorbed L1AnchorController into a L1State<P> type 2ebaf89 (this PR)
| .inner | ||
| .execute_transaction_without_commit((tx_env, recovered)); | ||
|
|
||
| self.evm_mut().reset_transaction_state(); |
There was a problem hiding this comment.
do we need to call it again here given that ZoneEvm::transact_raw handles it already?
There was a problem hiding this comment.
was just being extra careful, but i think u are right. On the flip side, i think it doesn't hurt?
83cb69c to
18a7ded
Compare
replaces #626
High-level goal
before this PR stack, Zones had two representations of Tempo L1 state:
PolicyCache/PolicyProvider, populated from typed TIP-403 and TIP-20 events with RPC fallback. CustomZoneTip20TokenandZoneTip403ProxyRegistryimplementations used it to reproduce upstream policy behavior.(address, slot), used byTempoStateand Zone system contracts for portal storage reads.this duplicated policy semantics and left two L1-state paths that had to remain aligned in block anchoring, event decoding, invalidation, RPC fallback, and upstream behavior. The goal is to remove the semantic policy implementation from the EVM path and execute upstream Tempo code against one factory-installed L1-aware database. The database wraps the normal caller-provided EVM database and:
TempoStatetransferPolicyIdfield in TIP-20’s packed slot, preserving Zone-local fieldsfrom upstream Tempo's perspective these remain ordinary EVM storage operations, but selected slots are virtualized to finalized L1 state. This makes upstream Tempo code the source of execution semantics rather than maintaining Zone-specific policy implementations or installing a separate storage provider around each precompile call.
EVM Database
database.rsaddsAnchoredZoneDb, a database adapter installed byZoneEvmFactoryfor every EVM created for payload building, canonical import, RPC calls, tracing, transaction-pool validation, and maintenance execution. The factory uses the alloy-evm database-context extension so the internal Tempo context can execute overAnchoredZoneDb<DB>whileZoneEvmcontinues to expose and return the original caller-providedDB.this keeps Reth’s state ownership and commit path unchanged. Reads pass through the adapter during execution, while successful state transitions are validated and sanitized before Reth commits them through the original database. Inner database failures preserve their original error type; anchor, L1-read, and mirrored-write failures are propagated as typed custom EVM errors.
the adapter and native
TempoStateprecompile share one execution-local anchor controller. It tracks whether execution is still at the parent Tempo anchor or has advanced to its direct child, rejects reads at an inconsistent anchor, rejects parent-anchor reads before advancement, and rejects duplicate or non-contiguous advancement. Noncommitting payload simulations restore the controller snapshot so they cannot leak anchor state into included transaction execution.until TIP-20 policy storage moves fully into TIP-403, the adapter retains compatibility with the current packed layout. It reads the local packed word first, replaces only the L1-owned
transferPolicyIdfield for execution, and restores the local field before canonical transitions are committed. TIP-403 account or storage mutations fail closed.Precompile Execution
execution.rsnow has one execution path for Zone-native and upstream Tempo precompiles. Every wrapper receives the same EVM configuration, storage-action recorder, and non-creditable-slot set. Whether a storage read is local or L1-backed is entirely a database concern rather than a choice between local and L1-aware precompile providers.CallRulescontains only Zone-specific admission behavior:Continueor ABI-encoded revert dataTIP-20, TIP-403, and
TipFeeManagernow execute the upstream Tempo implementations directly over ordinaryEvmPrecompileStorageProviderstorage. The remaining Zone-specific behavior is limited to privacy checks, bridge mint/burn authorization, read-only TIP-403 selectors, fixed gas, and delegate-call policy.this removes
ZonePrecompileStorageProvider, the separate local/L1-backed execution orchestration, and the custom L1-awareZoneFeeManager. Tempo’s ordinary protocol fee manager now observes the same anchored policy state through the EVM database adapter.Follow-up work
PolicyProvider#701